Repository navigation
fix: support addressed UDP datagrams and unblock DNS tests - #29
Conversation
284km
left a comment
284km
left a comment
There was a problem hiding this comment.
Reviewed head 7e3fde3. Found one Major issue: argument coercions can close the socket and let its descriptor be reused, after which the new send/receive paths perform I/O on the replacement socket. Please fix this before merging; the inline comment includes a reproducer and a proposed correction.
The payload-size check before the shared-arena copy and returning payload plus sender together are good choices. The bounded loopback regression covers binary and empty packets, multiple destinations, peeking, output buffers, and cooperative thread waits.
Validation on macOS, using an isolated snapshot of this head, its pinned mgz submodule, and the pinned Mere compiler 76023a6f:
- Rebuilt at O2.
unittest/udp_datagram_check.pymatched Ruby 4.0.9 with the hash index ON and OFF.run_corpus.sh --both: 496/496 programs matched, including the source/record checks that precede the corpus.- Additional dispatcher-name, namespace, method-write and regexp-capture checks passed;
git diff --checkpassed. - Targeted close-during-coercion probes reproduced both unintended sending and consumption of another socket's queued packet, with the hash index ON and OFF. Ruby 4.0.9 raises
IOErrorand leaves the replacement socket untouched.
The upstream DNS measurement and Linux execution were not repeated locally.
| " loop do\n" ++ | ||
| " n = Socket.__udp_sendto(@__io_fd, mesg, flags, host, port)\n" ++ |
There was a problem hiding this comment.
[Major] Recheck the socket after argument coercions
The initial __check_open runs before conversions that may execute Ruby code (to_str, to_int, and destination conversions). A conversion can close this socket and open another one. Closing retains the numeric @__io_fd, so if the descriptor is reused, this first native call sends through the replacement socket. The check after the readiness wait does not protect the first call.
For example:
require 'socket'
receiver = UDPSocket.new
receiver.bind('127.0.0.1', 0)
udp = UDPSocket.new
replacement = nil
flags = Object.new
flags.define_singleton_method(:to_int) do
udp.close
replacement = UDPSocket.new
0
end
udp.send('x', flags, '127.0.0.1', receiver.addr[1])Ruby 4.0.9 raises IOError: closed stream. This head returns 1, and the receiver gets 'x' from the replacement socket.
The same ordering appears in __udp_packet: flags/maxlen coercions run after its open check. I also reproduced recvfrom consuming a queued packet belonging to the replacement socket. Both cases reproduce in the O2 build with the hash index ON and OFF.
Please repeat __check_open after all conversions and before the first native call in both paths, and add regressions that close and reuse the descriptor from conversion callbacks.
There was a problem hiding this comment.
Fixed in d93c187. Both UDP paths now recheck __check_open at the top of the native-call loop, after all argument conversions and before the first syscall. This also retains the recheck after a readiness wait.
Added regressions that close the original socket and verify descriptor reuse from message, flags, host, service, and packed-address conversions. Receive regressions cover both recv and recvfrom, with length and flags conversions. They verify IOError: closed stream, no unexpected packet at the send destination, and an untouched queued packet on the replacement receiver.
The expanded regression fails on the pre-fix O2 binary and matches Ruby 4.0.9 in both hash-index modes on macOS and Linux after the fix.
| | Cons (VInt fd, Cons (VStr h, Cons (VInt fl, Cons (host, Cons (VInt port, Nil))))) -> | ||
| (let data = sget h in | ||
| let n = str_len data in | ||
| if n > 65536 then raise_exc world "Errno::EMSGSIZE" "Message too long - sendto(2)" |
There was a problem hiding this comment.
Good boundary check: rejecting an oversized datagram before copying any bytes protects the shared 64 KiB arena for both addressed and connected sends. The existing oversized-payload regression exercises this refusal.
7e3fde3 to
d93c187
Compare
resolv/test_dns.rbstalled when its unconnected UDP sender raisedNotImplementedErrorand a peer thread remained waiting. The saved CI measurement reached its 600-second bound (606.747 seconds including supervision); this head completes all 39 tests locally in 17.150 seconds under the same 600-second/2 GiB bounds.Implement addressed
UDPSocket#sendand sender-awarerecvfromusing native datagram operations. Support host/port, packed sockaddr, and Addrinfo destinations; preserve the descriptor and binding during sends; return each packet's actual sender; and keep binary data, empty datagrams, MSG_PEEK, output buffers, conversion errors, and cooperative readiness waits. Oversized payloads are rejected before copying into the shared arena. Both paths recheck that the socket is open after argument conversions and before every native call, so a conversion cannot cause I/O on a reused descriptor.Rebased onto main after #18, #20, #25, #26, #27, and #28 were merged. The prerequisite merelang/mere#32 is now merged. CI pins its published commit
991f299b20f18639713685fe3297e087ddc5b1d7, which includes the v0.1.711 compiler changes required by main and the zero-capacity receive correction. CI on this head passed all 29 jobs; all 758 CRuby test files were collected from 8 shards with no missing files or infrastructure errors. This confirms complete measurement, not full Ruby conformance.Validation of
d93c187:git diff --checkpass.Socket.ip_address_listAPI intest_oversized_name_is_rescuable_as_resolv_error. Peak process-family RSS was 514 MiB.This removes one observed file-level stall; the saved baseline used an earlier compiler/source revision, so these timings do not establish a whole-workflow speedup. UDP connect still replaces the descriptor, so retaining a prior binding across connect, generic Socket datagram paths, and other socket API gaps remain outside this change.